-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CST leading and trailing edge fixes and improvements #252
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 65.35% 65.42% +0.06%
==========================================
Files 47 47
Lines 12286 12307 +21
==========================================
+ Hits 8030 8052 +22
+ Misses 4256 4255 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look like very useful updates, thanks Sabet! I took a first look and have a few comments/questions.
Also since the interface changes, does this merit a version update?
pygeo/parameterization/DVGeoCST.py
Outdated
self.foilCoords = self.foilCoords[:-1, :] | ||
|
||
# Flip the y-coordinates to counter-clockwise if they are clockwise | ||
if self.foilCoords[0, self.yIdx] < self.foilCoords[-1, self.yIdx]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this check break if the airfoil is sharp? Might be better to check indices 1
and -2
if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually works for sharp airfoils because of the check above it that removes duplicate points
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many airfoils (e.g., the RAE 2822) have a lower surface slope at the TE that makes the second-to-last point on the lower surface actually has a positive y value. So it might think the lower surface is the upper surface. Would that break this check?
I'm trying to think of all the corner cases and come up with a potentially more robust method. Checking the second to last would work if the x coordinates of all the upper/lower points are the same, but maybe there's some edge case where given different upper and lower x coordinates the y values will make this check wrong. Maybe there's some way to take a vector between two points on each surface at the TE and do some geometry with it? You might be able to use the sign of the Z component of the cross product between the vectors at the TE (idx 1 - 0 and idx -1 - -2) as a check. I haven't thought it through fully, but there's gotta be something simple and robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are both cases where the current check could break. The cross-product approach should work
Is this ready to go and/or be reviewed by others? |
Yes, this is ready for both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I was not familiar with this part of the codebase so I had to learn it on the fly. Great job, especially with the updated tests!!
Thanks for the reviews! |
Purpose
I made a few fixes and improvements to how the CST implementation handles leading and trailing edges:
_splitUpperLower
._splitUpperLower
associates each point with either the upper or lower surface. This makes sense foraddPointSet
calls but not for the CST fit. When the airfoil has a single LE point or a sharp TE, those points should be included in both the upper and lower surfaces. This change ensures that symmetric airfoils have symmetric CST coefficients.computeCSTCoordinates
in the fit error check and intest_fitCST
plotCST
Expected time until merged
Not urgent, 2-3 weeks would be nice
Type of change
Testing
To verify the code changes, I:
Kulfan
class in pyESP and added regression tests for the coefficients (the reference is still DVGeometryCST)Here is an example of how the CST coefficients change with this PR (for a NACA 0012 airfoil with leading edge at 0,0). The fit error computed by the internal check is lower, the upper and lower surface coefficients are symmetric, and the coefficients match pyESP.
naca0012_zeroLE.dat on main branch
naca0012_zeroLE.dat on PR branch
naca0012_zeroLE.dat with pyESP
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable